Bug in iloc aligned objects #37728

phofl · 2020-11-10T00:27:19Z

closes Replacing multiple columns (or just one) with iloc does not work #22046
closes BUG: DataFrame iloc does not accept Series assignment #37593
tests added / passed
passes black pandas
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

If iloc should be purely position based, this PR fixes the bug described in the issue. One test depends on the wrong behavior, so I had to adjust it.

pandas/core/indexing.py

� Conflicts: � pandas/tests/indexing/test_iloc.py

jbrockmendel · 2020-11-12T00:12:03Z

pandas/core/indexing.py

+        if name == "iloc":
+            for index, loc in zip(range(len(ilocs)), ilocs):
+                val = value.iloc[:, index]
+                self._setitem_single_column(loc, val, plane_indexer)


can this be solved with an appropriate call to _align_frame or _align_series? id really prefer to avoid passing name around. by the time we get to setitem_with_indexer we should always be working with positional indexers (i.e. iloc)

Hm no not to my knowledge. The issue is, that we want to achieve oposing things between loc and iloc, but the objects do look the same. setitem_with_indexer could be solved with a default value, that is not a problem.

But I can not see a way to do this without passing some kind of flag to the align calls.

Looked into this a bit again. The indexer in setitem_with_indexer already is positional, but the value may not be properly aligned for loc, hence we call the align functions there. This is not necessary for the iloc path, but this information is lost there, if we do not pass it through somehow.

it sounds like we should be doing some appropriate alignment within Loc before passing it into iLoc?

That would be an option, but would result in a bigger rewrite and I am not sure if that would introduce more complexity, because the align_series method is called in various places. Should I give it a try?

it looks like this PR is making it so that we never call align_frame/align_series if setitem_with_indexer was called from iloc, only if it was called from loc. setitem_with_indexer is only called from one place within loc. Can we just do that alignment right before that call?

Hm, we could try, but it is not that simple. For example before calling setitem_with_indexer from loc, value may be a DataFrame with different dtypes. Currently _align_series is called for every column to get a ndarray with the correct dtype. If we call this before calling setitem_with_indexer we would have to call _align_frame or copy the logic, which decides if we align every colum at once or separately. _align_frame returns an object dtype array, which obviously would introduce a lot of dtype errors. That is what I meant with a bigger rewrite here.

thanks for explaining, i think i have a better handle on it now.

ive got an upcoming PR to make all 2D cases go through split_path. let's revisit this after that goes through and see if we can make it cleaner

That sounds great. Just ping me when merged, then I will have a look.

pandas/tests/indexing/test_indexing.py

pandas/core/frame.py

pandas/core/indexing.py

� Conflicts: � doc/source/whatsnew/v1.2.0.rst

pep8speaks · 2020-11-17T21:12:48Z

Hello @phofl! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-11-18 20:26:46 UTC

pandas/core/frame.py

pandas/tests/frame/indexing/test_setitem.py

pandas/tests/series/indexing/test_setitem.py

jbrockmendel · 2020-11-17T22:16:21Z

pandas/tests/frame/indexing/test_setitem.py

+        # GH: 22046
+        df1 = DataFrame({"a2": [11, 12, 13], "b2": [14, 15, 16]})
+        df2 = DataFrame({"a": [1, 2, 3], "b": [4, 5, 6], "c": [7, 8, 9]})
+        df2.iloc[:, [1]] = df1.iloc[:, [0]]


are there other indexer/value type combinations that are relevant here? e.g. the value being set here is a DataFrame, would a Series trigger the same bug? what if instead of [1] the indexer was slice(1, 2)?

df1 = DataFrame({"a2": [11, 12, 13], "b2": [14, 15, 16]}) df2 = DataFrame({"a": [1, 2, 3], "b": [4, 5, 6], "c": [7, 8, 9]}) df2.iloc[:, [1]] = df1.iloc[:, 0]

raises

Traceback (most recent call last): File "/home/developer/.config/JetBrains/PyCharm2020.2/scratches/scratch_4.py", line 132, in <module> df2.iloc[:, [1]] = df1.iloc[:, 0] File "/home/developer/PycharmProjects/pandas/pandas/core/indexing.py", line 684, in __setitem__ iloc._setitem_with_indexer(indexer, value) File "/home/developer/PycharmProjects/pandas/pandas/core/indexing.py", line 1637, in _setitem_with_indexer self._setitem_single_block(indexer, value) File "/home/developer/PycharmProjects/pandas/pandas/core/indexing.py", line 1851, in _setitem_single_block self.obj._mgr = self.obj._mgr.setitem(indexer=indexer, value=value) File "/home/developer/PycharmProjects/pandas/pandas/core/internals/managers.py", line 562, in setitem return self.apply("setitem", indexer=indexer, value=value) File "/home/developer/PycharmProjects/pandas/pandas/core/internals/managers.py", line 427, in apply applied = getattr(b, f)(**kwargs) File "/home/developer/PycharmProjects/pandas/pandas/core/internals/blocks.py", line 1005, in setitem values[indexer] = value ValueError: shape mismatch: value array of shape (3,) could not be broadcast to indexing result of shape (1,3) Process finished with exit code 1

Did not check slice previously, this triggered the bug too. Will add test

jbrockmendel · 2020-11-17T22:18:40Z

pandas/core/indexing.py

@@ -1816,13 +1821,13 @@ def _setitem_single_block(self, indexer, value):

            indexer = maybe_convert_ix(*indexer)

-        if isinstance(value, (ABCSeries, dict)):
+        if isinstance(value, (ABCSeries, dict)) and name != "iloc":


if we have a dict here dont we still want to wrap it in a Series?

Yep, have no tests getting here with iloc. Will add one

LMK when these tests are all ready and ill take another look. this one is almost ready

Tests are all in

jreback

lgtm. minor comments can be a followup. @jbrockmendel merge when good.

jreback · 2020-11-18T00:57:24Z

pandas/core/indexing.py

@@ -1714,7 +1714,7 @@ def _setitem_with_indexer_2d_value(self, indexer, value):
            # setting with a list, re-coerces
            self._setitem_single_column(loc, value[:, i].tolist(), pi)

-    def _setitem_with_indexer_frame_value(self, indexer, value: "DataFrame"):
+    def _setitem_with_indexer_frame_value(self, indexer, value: "DataFrame", name):


can you type name here (and above)

jreback · 2020-11-18T00:57:47Z

pandas/core/indexing.py

@@ -1787,7 +1792,7 @@ def _setitem_single_column(self, loc: int, value, plane_indexer):
        # reset the sliced object if unique
        self.obj._iset_item(loc, ser)

-    def _setitem_single_block(self, indexer, value):
+    def _setitem_single_block(self, indexer, value, name):


pandas/core/indexing.py

jbrockmendel · 2020-11-18T03:27:49Z

pandas/core/indexing.py

@@ -1854,7 +1858,7 @@ def _setitem_with_indexer_missing(self, indexer, value):
            if index.is_unique:
                new_indexer = index.get_indexer([new_index[-1]])
                if (new_indexer != -1).any():
-                    return self._setitem_with_indexer(new_indexer, value)
+                    return self._setitem_with_indexer(new_indexer, value, name)


double check me on this, but i think it might be the case that we only get here with name = "loc"

Yep, you are right. Iloc raises when indexer is a dict. That is the only way we can get in there. Removed it

pandas/core/indexing.py

jreback · 2020-11-19T00:22:11Z

thanks @phofl nice!

jbrockmendel · 2020-11-19T19:56:07Z

pandas/tests/indexing/test_iloc.py

+        expected = DataFrame({"a": [1, 2, 3], "b": [11, 12, 13], "c": [7, 8, 9]})
+        tm.assert_frame_equal(df2, expected)
+
+    def test_setitem_iloc_dictionary_value(self):


nitpick for future: setitem_iloc -> iloc_setitem

jbrockmendel · 2020-11-19T19:56:41Z

agreed thanks @phofl, this wasnt easy, glad to see it fixed

jbrockmendel · 2020-11-19T19:58:08Z

pandas/core/indexing.py

@@ -1815,14 +1821,13 @@ def _setitem_single_block(self, indexer, value):
                return

            indexer = maybe_convert_ix(*indexer)
-
-        if isinstance(value, (ABCSeries, dict)):
+        if isinstance(value, ABCSeries) and name != "iloc" or isinstance(value, dict):


for follow-up, can you put parens where appropriate so i dont have to think about whether the "and" gets evaluated before the "or"

jbrockmendel · 2020-11-19T19:58:48Z

pandas/core/indexing.py

@@ -1854,7 +1859,7 @@ def _setitem_with_indexer_missing(self, indexer, value):
            if index.is_unique:
                new_indexer = index.get_indexer([new_index[-1]])
                if (new_indexer != -1).any():
-                    return self._setitem_with_indexer(new_indexer, value)
+                    return self._setitem_with_indexer(new_indexer, value, "loc")


comment explaining that we only get here with name=="loc" and thats why its hardcoded (or assertion)

phofl · 2020-11-19T20:00:27Z

Thanks @jbrockmendel and @jreback
Will add a followup to adress comments.

Bug in iloc aligned objects

9d11511

phofl added the Indexing Related to indexing on series/frames, not to indexes themselves label Nov 10, 2020

phofl mentioned this pull request Nov 10, 2020

BUG: DataFrame iloc does not accept Series assignment #37593

Closed

2 tasks

jbrockmendel reviewed Nov 10, 2020

View reviewed changes

pandas/core/indexing.py Outdated Show resolved Hide resolved

jbrockmendel reviewed Nov 10, 2020

View reviewed changes

pandas/core/indexing.py Outdated Show resolved Hide resolved

phofl added 4 commits November 10, 2020 20:29

Pass name through functions

ca3f47b

Fix dtype

f1306aa

Add test

c55cd62

Merge branch 'master' of https://github.com/pandas-dev/pandas into 22046

ea41407

� Conflicts: � pandas/tests/indexing/test_iloc.py

jbrockmendel reviewed Nov 12, 2020

View reviewed changes

jbrockmendel reviewed Nov 14, 2020

View reviewed changes

pandas/tests/indexing/test_indexing.py Show resolved Hide resolved

jbrockmendel reviewed Nov 16, 2020

View reviewed changes

pandas/core/frame.py Outdated Show resolved Hide resolved

jbrockmendel reviewed Nov 17, 2020

View reviewed changes

pandas/core/indexing.py Outdated Show resolved Hide resolved

phofl added 3 commits November 17, 2020 22:10

Default name to iloc

6bced6a

Simplify for loop

689a1ab

Merge branch 'master' of https://github.com/pandas-dev/pandas into 22046

d870dd6

� Conflicts: � doc/source/whatsnew/v1.2.0.rst

phofl added 3 commits November 17, 2020 22:13

Delete spaces

23dab4a

Rename plane indexer

48e0d25

Merge branch 'master' of https://github.com/pandas-dev/pandas into 22046

ea068ba

jbrockmendel reviewed Nov 17, 2020

View reviewed changes

pandas/core/frame.py Outdated Show resolved Hide resolved

jbrockmendel reviewed Nov 17, 2020

View reviewed changes

pandas/tests/frame/indexing/test_setitem.py Outdated Show resolved Hide resolved

Set to default

8ff2430

jbrockmendel reviewed Nov 17, 2020

View reviewed changes

pandas/tests/series/indexing/test_setitem.py Outdated Show resolved Hide resolved

jbrockmendel reviewed Nov 17, 2020

View reviewed changes

Move tests

8a697f7

jbrockmendel reviewed Nov 17, 2020

View reviewed changes

phofl added 3 commits November 17, 2020 23:25

Add test

3bef35a

Add test for dict case

7375f22

Add tm

e58fbc8

jreback added this to the 1.2 milestone Nov 18, 2020

jreback added the Bug label Nov 18, 2020

jreback approved these changes Nov 18, 2020

View reviewed changes

jbrockmendel reviewed Nov 18, 2020

View reviewed changes

pandas/core/indexing.py Show resolved Hide resolved

phofl added 3 commits November 18, 2020 21:06

Type and refactor whatsnew

55e9403

Add comment

7016977

Remove name in function

b965eee

jreback merged commit 3d259e6 into pandas-dev:master Nov 19, 2020

phofl deleted the 22046 branch November 19, 2020 15:43

jbrockmendel reviewed Nov 19, 2020

View reviewed changes

phofl mentioned this pull request Jan 20, 2021

BUG: iloc assignment in Pandas 1.2.0 #39261

Closed

3 tasks

simonjayhawkins mentioned this pull request Feb 22, 2021

BUG: Index alignment behaviour #39931

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug in iloc aligned objects #37728

Bug in iloc aligned objects #37728

phofl commented Nov 10, 2020 •

edited

Loading

jbrockmendel Nov 12, 2020

phofl Nov 12, 2020

phofl Nov 13, 2020

jbrockmendel Nov 14, 2020

phofl Nov 14, 2020

jbrockmendel Nov 16, 2020

phofl Nov 16, 2020

jbrockmendel Nov 17, 2020

phofl Nov 17, 2020

pep8speaks commented Nov 17, 2020 •

edited

Loading

jbrockmendel Nov 17, 2020

phofl Nov 17, 2020

jbrockmendel Nov 17, 2020

phofl Nov 17, 2020

jbrockmendel Nov 18, 2020

phofl Nov 18, 2020

jreback left a comment

jreback Nov 18, 2020

phofl Nov 18, 2020

jreback Nov 18, 2020

phofl Nov 18, 2020

jbrockmendel Nov 18, 2020

phofl Nov 18, 2020

jreback commented Nov 19, 2020

jbrockmendel Nov 19, 2020

jbrockmendel commented Nov 19, 2020

jbrockmendel Nov 19, 2020

jbrockmendel Nov 19, 2020

phofl commented Nov 19, 2020

Bug in iloc aligned objects #37728

Bug in iloc aligned objects #37728

Conversation

phofl commented Nov 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pep8speaks commented Nov 17, 2020 • edited Loading

Comment last updated at 2020-11-18 20:26:46 UTC

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Nov 19, 2020

Choose a reason for hiding this comment

jbrockmendel commented Nov 19, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phofl commented Nov 19, 2020

phofl commented Nov 10, 2020 •

edited

Loading

pep8speaks commented Nov 17, 2020 •

edited

Loading